-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changing update list to update list values along with the capacity. #3897
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Build Failed 😱 Build Id: 5b639d5e-252c-447e-ba3d-24a6e928ac47 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
@igooch This built locally but it looks like the cloud build failed. I am unable to view build:
|
Looks like it's failing on
|
You need to be in https://groups.google.com/a/google.com/g/agones-discuss to get access to the build logs. Also looks like you're missing the CLA. |
I am looking at that now and I see the following:
If I am not mistaken it looks like the test is expecting the return to have values associated with it. In all my testing of the agones SDK I have not been able to get any update list call to return the correct data. I think this is due to the fact that this gets put into a queue to get processed asynchronously, for example RemoveListValue also returns an empty object. Would adjusting the test to match this pattern be a reasonable approach? |
Build Failed 😱 Build Id: e76e1f1a-1713-4f6a-b56f-b9f54d05d847 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
The test is failing on lines These are checking that the values from GetList and the K8s API are as expected after the update. An important line in the test is agones/pkg/sdkserver/sdkserver_test.go Lines 1797 to 1800 in 31ed93e
agones/pkg/sdkserver/sdkserver_test.go Lines 1805 to 1809 in 31ed93e
From taking a quick look at your code it doesn't use the fieldMask values. You can take a look at the localsdkserver code which uses the field masks. |
/kind bug
What this PR does / Why we need it:
This PR updates the UpdateList functionality to update the list values along side the given capacity. This is very important because without it managing game server keys gets very complicated and often causes unwanted race conditions when sending multiple additions or removals to lists in quick succession.
Which issue(s) this PR fixes:
Closes #3870